fix: Fix optional doubles not building in Release mode on Swift#1065
fix: Fix optional doubles not building in Release mode on Swift#1065
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
This is where we have this case already: Why didn't this Maybe we need to actually access |
…f that fails in release
|
I just added a test for this that passes the following struct: interface PartialPerson {
age?: number
name?: string
}..to native, and native Swift accesses I am running this test two times:
I built this in release, once with the code before: var age: Double? {
@inline(__always)
get {
return self.__age.value
}..and once with the code after the change suggested by @hyochan in the thread over at his repo: var age: Double? {
@inline(__always)
get {
return self.__age.hasValue ? self.__age.pointee : nil
}In both cases, the tests run fine without any corruption or errors in release mode: So we need another way to actually reproduce this. Are you guys maybe on an old Xcode version? I'm on Version 26.1.1 (17B100). |
|
I think there is still issue even we applied nitro patch in #746. Our Test Environment
Our Test ResultsWe tested with the AudioSet configuration in JS: const audioSet = {
AVSampleRateKeyIOS: 22050,
AVNumberOfChannelsKeyIOS: 2,
AudioSamplingRate: 22050,
AudioChannels: 2,
AudioEncodingBitRate: 64000,
};Release build recording result: Expected: 2 channels, 22050 Hz The patch did NOT fix the issue for us in Release builds. Key Difference: Struct ComplexityYour
Our
{
"AudioSet": {
"AudioEncoderAndroid": "AudioEncoderAndroidType?",
"OutputFormatAndroid": "OutputFormatAndroidType?",
"AudioSourceAndroid": "AudioSourceAndroidType?",
"AVEncoderAudioQualityKeyIOS": "AVEncoderAudioQualityIOSType?",
"AVModeIOS": "AVModeIOSOption?",
"AVEncodingOptionIOS": "AVEncodingOption?",
"AVFormatIDKeyIOS": "AVEncodingOption?",
"AVNumberOfChannelsKeyIOS": "double?",
"AVSampleRateKeyIOS": "double?",
"AVLinearPCMBitDepthKeyIOS": "AVLinearPCMBitDepthKeyIOSType?",
"AVLinearPCMIsBigEndianKeyIOS": "boolean?",
"AVLinearPCMIsFloatKeyIOS": "boolean?",
"AVLinearPCMIsNonInterleavedIOS": "boolean?",
"AudioQuality": "AudioQualityType?",
"AudioChannels": "double?",
"AudioSamplingRate": "double?",
"AudioEncodingBitRate": "double?",
"IncludeBase64": "boolean?"
}
}Possible Causes
Suggestion for ReproductionCould you try a more complex struct similar to ours? interface ComplexStruct {
prop1?: number // double?
prop2?: number
prop3?: number
prop4?: number
prop5?: SomeEnum // enum type
prop6?: SomeEnum
prop7?: number
prop8?: number
prop9?: boolean
prop10?: boolean
prop11?: number
prop12?: number
}And access multiple properties in sequence on the native side (not just one property round-trip). How to Reproduce with Our Repogit clone https://github.com/hyochan/react-native-nitro-sound.git
cd react-native-nitro-sound
yarn install
cd example
yarn ios:pod
yarn build:ios # Release build
# Run on simulator, go to "NitroSound with Hook" screen, record audio
# Check recorded file with: afinfo <path_to_m4a_file>The recorded file should show 22050 Hz, 2 channels if working correctly. |
Can you try this? Fork this repo and just edit the structs - it's quite easy to build the example app. I'm on vacation now
|
|
I think if you can reproduce this you should probably create a bug report in the swiftlang/swift repo. |
I've tested current PR directly in our react-native-nitro-sound repo. Test Setup:
Result:afinfo output: Key Observation:The generated AudioSet.swift shows mixed patterns:
Our struct has 18 optional properties (vs your PartialPerson test with 2) |
Done in swiftlang/swift#85735. Have nice vacation! |


Fixes a bug mentioned by hyochan/react-native-nitro-sound#746 (comment) where a
number | undefined(std::optional<double>) would not build in Release mode on Swift.The fix is to not use the Swift generated
.valueaccessor, but instead do a.has_value() ? .pointer : nilaccess.The problem is, we use
std::optional<double>in thePersonstruct in the example/test module, and it builds just fine in Release mode.So this cannot be blindly merged until we have an actual test in this repo demonstrating this exact problem. cc @hyochan let me know if you find something